-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(zetacore): delete ballots after maturity #2863
base: develop
Are you sure you want to change the base?
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduced in this pull request focus on enhancing ballot management within the system, specifically addressing the deletion of matured ballots. This includes the addition of new message types, modifications to existing functions for handling ballots, and the introduction of event emissions upon ballot deletion. The updates aim to streamline the lifecycle management of ballots, ensuring that matured ballots are efficiently removed from the state to optimize resource use. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Keeper
participant EventManager
User->>Keeper: Request to delete matured ballots
Keeper->>Keeper: Identify matured ballots
Keeper->>Keeper: Delete matured ballots
Keeper->>EventManager: Emit EventBallotDeleted
EventManager-->>Keeper: Confirmation of event emission
Assessment against linked issues
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2863 +/- ##
===========================================
+ Coverage 61.71% 61.79% +0.07%
===========================================
Files 440 441 +1
Lines 31141 31235 +94
===========================================
+ Hits 19220 19302 +82
- Misses 11058 11066 +8
- Partials 863 867 +4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Outside diff range and nitpick comments (6)
x/emissions/types/expected_keepers.go (1)
19-19
: Consider enhancing the clarity and documentation of theClearMaturedBallots
method.The addition of the
ClearMaturedBallots
method to theObserverKeeper
interface is a good change as it allows for the clearing of matured ballots. However, consider the following suggestions to improve the clarity and documentation of the method:
Consider renaming the method to something more specific, such as
RemoveMaturedBallotsFromState
, to clearly indicate that it's removing the ballots from the state.Consider renaming the
maturityBlocks
parameter to a more descriptive name, such asmaturityThreshold
, to clarify its purpose.Add a comment above the method to explain its functionality and any important details about its usage. This will help future maintainers understand the purpose and behavior of the method.
x/observer/keeper/events_test.go (1)
15-37
: Consider defining the test case struct outside the test function for better readability.The test case struct is defined within the test function, which can make the test function harder to read. Consider moving the struct definition outside the test function.
-func TestEmitEventBallotDeleted(t *testing.T) { - tt := []struct { - name string - ballotIdentifier string - ballotType types.ObservationType - voters []string - voteType []types.VoteType - }{ - { - name: "successfull votes only", - ballotIdentifier: sample.ZetaIndex(t), - ballotType: types.ObservationType_InboundTx, - voters: []string{"voter1", "voter2"}, - voteType: []types.VoteType{types.VoteType_SuccessObservation, types.VoteType_SuccessObservation}, - }, - { - name: "failed votes only", - ballotIdentifier: sample.ZetaIndex(t), - ballotType: types.ObservationType_InboundTx, - voters: []string{"voter1", "voter2"}, - voteType: []types.VoteType{types.VoteType_FailureObservation, types.VoteType_FailureObservation}, - }, - } +type testCase struct { + name string + ballotIdentifier string + ballotType types.ObservationType + voters []string + voteType []types.VoteType +} + +func TestEmitEventBallotDeleted(t *testing.T) { + tt := []testCase{ + { + name: "successfull votes only", + ballotIdentifier: sample.ZetaIndex(t), + ballotType: types.ObservationType_InboundTx, + voters: []string{"voter1", "voter2"}, + voteType: []types.VoteType{types.VoteType_SuccessObservation, types.VoteType_SuccessObservation}, + }, + { + name: "failed votes only", + ballotIdentifier: sample.ZetaIndex(t), + ballotType: types.ObservationType_InboundTx, + voters: []string{"voter1", "voter2"}, + voteType: []types.VoteType{types.VoteType_FailureObservation, types.VoteType_FailureObservation}, + }, + }x/observer/keeper/grpc_query_ballot_test.go (1)
Line range hint
127-132
: Approved with SuggestionsThe code changes are minimal and do not introduce any issues. However, consider the following suggestions to improve the code:
Improve Error Handling: In the 3rd sub-test, use
require.ErrorIs
to check for a specific error type instead of justrequire.Error
. This provides a more precise assertion.Simplify
Voters
Field: TheVoters
field in the expected response can be simplified by using a slice literal instead of a slice of structs. This improves readability and reduces verbosity.- Voters: []types.VoterList{ - { - VoterAddress: voter, - VoteType: types.VoteType_SuccessObservation, - }, - }, + Voters: []types.VoterList{{voter, types.VoteType_SuccessObservation}},x/observer/keeper/ballot_test.go (1)
211-310
: Suggestions for improvement:The
TestKeeper_ClearMaturedBallots
function is well-structured and covers various scenarios. However, consider the following suggestions to enhance the test:
- Extract the hardcoded
numberOfBallots
value as a constant or use a random number to ensure the tests are more robust.- Extract the event counting logic into a separate helper function to avoid duplication in the first and third sub-tests.
- Use
append
instead of initializing theballots
slice with a fixed size based onnumberOfBallots
to make the code more flexible.x/emissions/abci_test.go (2)
Line range hint
206-330
: Consider refactoring the test for better readability, robustness, and performance.While the test covers the core functionality well, consider the following improvements:
Split the test into smaller, focused sub-tests using
t.Run()
for better readability and maintainability. Each sub-test can cover a specific scenario, such as successful distribution, edge cases, and error scenarios.Add more assertions to cover edge cases and error scenarios, such as:
- Testing with an empty observer set.
- Testing with no ballots.
- Testing with insufficient funds in the emission pool.
- Testing with invalid parameters.
Optimize the test by reducing the number of blocks (
numberOfTestBlocks
) to the minimum required to cover the scenarios. This will make the test run faster without compromising the coverage.Use more concise assertions by leveraging helper functions or libraries like
testify/require
. This will make the test more readable and maintainable.
Line range hint
332-487
: Consider enhancing the test for better robustness and maintainability.The test is well-structured and covers the important scenarios. The use of table-driven tests is a good practice. However, consider the following improvements:
Add assertions for error scenarios, such as:
- Testing with an invalid observer set.
- Testing with invalid ballots.
- Testing with insufficient funds in the emission pool.
Extract the common setup code into a helper function to avoid duplication and improve readability. The setup code includes:
- Creating the keeper instances.
- Setting up the observer set and ballots.
- Funding the emission pool.
- Setting the parameters.
This will make the test more maintainable and easier to extend with new scenarios.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (6)
typescript/zetachain/zetacore/observer/ballot_pb.d.ts
is excluded by!**/*_pb.d.ts
typescript/zetachain/zetacore/observer/events_pb.d.ts
is excluded by!**/*_pb.d.ts
typescript/zetachain/zetacore/observer/query_pb.d.ts
is excluded by!**/*_pb.d.ts
x/observer/types/ballot.pb.go
is excluded by!**/*.pb.go
,!**/*.pb.go
x/observer/types/events.pb.go
is excluded by!**/*.pb.go
,!**/*.pb.go
x/observer/types/query.pb.go
is excluded by!**/*.pb.go
,!**/*.pb.go
Files selected for processing (16)
- changelog.md (1 hunks)
- proto/zetachain/zetacore/observer/ballot.proto (1 hunks)
- proto/zetachain/zetacore/observer/events.proto (2 hunks)
- proto/zetachain/zetacore/observer/query.proto (1 hunks)
- testutil/keeper/mocks/emissions/observer.go (1 hunks)
- x/emissions/abci.go (3 hunks)
- x/emissions/abci_test.go (4 hunks)
- x/emissions/types/expected_keepers.go (1 hunks)
- x/observer/keeper/ballot.go (3 hunks)
- x/observer/keeper/ballot_test.go (1 hunks)
- x/observer/keeper/events.go (1 hunks)
- x/observer/keeper/events_test.go (1 hunks)
- x/observer/keeper/grpc_query_ballot.go (1 hunks)
- x/observer/keeper/grpc_query_ballot_test.go (1 hunks)
- x/observer/types/ballot.go (1 hunks)
- x/observer/types/ballot_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- changelog.md
Additional context used
Path-based instructions (15)
x/emissions/types/expected_keepers.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.proto/zetachain/zetacore/observer/ballot.proto (1)
Pattern
**/*.proto
: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/observer/keeper/grpc_query_ballot.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.proto/zetachain/zetacore/observer/events.proto (1)
Pattern
**/*.proto
: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/observer/keeper/events.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/events_test.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.testutil/keeper/mocks/emissions/observer.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/ballot.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/types/ballot.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/grpc_query_ballot_test.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/emissions/abci.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/observer/keeper/ballot_test.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.proto/zetachain/zetacore/observer/query.proto (1)
Pattern
**/*.proto
: Review the Protobuf definitions, point out issues relative to compatibility, and expressiveness.x/observer/types/ballot_test.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.x/emissions/abci_test.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Additional comments not posted (19)
proto/zetachain/zetacore/observer/ballot.proto (1)
45-48
: LGTM!The
VoterList
message is well-structured and follows the protocol buffer conventions. The field names are descriptive and the types are appropriate. The use of the existingVoteType
enum for thevote_type
field ensures type safety.This message definition aligns with the PR objectives of tracking voter information and can be effectively used to represent a voter's address and their corresponding vote type.
x/observer/keeper/grpc_query_ballot.go (1)
46-46
: Refactoring Approved: Voter List Generation Delegated to a Separate MethodThe refactoring of the voter list generation logic into the
GenerateVoterList
method improves code clarity and maintainability. It encapsulates the complexity of constructing the voter list, reducing the potential for errors in theBallotByIdentifier
function.However, to ensure a comprehensive review, it is recommended to also review the implementation of the
GenerateVoterList
method to verify its correctness and efficiency.proto/zetachain/zetacore/observer/events.proto (2)
7-7
: LGTM!The import statement is necessary for using the
VoterList
message in theEventBallotDeleted
message.
19-24
: LGTM!The
EventBallotDeleted
message is well-defined and captures the necessary information for a ballot deletion event. The use ofrepeated
and(gogoproto.nullable) = false
for thevoters
field is a good practice to ensure data integrity.x/observer/keeper/events_test.go (1)
77-79
: LGTM!The
RemoveQuotes
function is simple and straightforward. The code changes are approved.testutil/keeper/mocks/emissions/observer.go (1)
18-21
: LGTM!The new mock function
ClearMaturedBallots
is correctly implemented. It properly registers the invocation with the provided arguments using theCalled
method from themock.Mock
library, which is the expected behavior for a mock function.x/observer/keeper/ballot.go (5)
23-26
: LGTM!The
DeleteBallot
function is implemented correctly and follows the standard pattern for deleting data from the store using a prefix store. The code changes are approved.
28-31
: LGTM!The
DeleteBallotList
function is implemented correctly and follows the standard pattern for deleting data from the store using a prefix store. The code changes are approved.
54-54
: LGTM!The changes to the
GetMaturedBallots
function improve the clarity and modularity of the code by separating the height calculation logic into a new helper functionGetMaturedBallotHeight
. The code changes are approved.
79-88
: LGTM!The
ClearMaturedBallots
function is well-structured and encapsulates the logic for clearing matured ballots effectively. It iterates over the list of ballots, deletes each one using theDeleteBallot
method, and emits an event for each deletion. It also deletes the ballot list corresponding to the matured height using theDeleteBallotList
method. The code changes are approved.
90-92
: LGTM!The
GetMaturedBallotHeight
function is a simple and clear implementation of the height calculation logic. It improves the readability and modularity of the code by separating this logic from theGetMaturedBallots
function. The code changes are approved.x/observer/keeper/grpc_query_ballot_test.go (1)
Line range hint
14-80
: LGTM!The
TestKeeper_HasVoted
function is well-structured and covers important scenarios. The code changes are approved.x/emissions/abci.go (3)
13-13
: LGTM!The import statement is necessary and correctly added.
106-112
: LGTM!The code changes for initializing the
ballots
slice and accumulating the ballots are correctly implemented.
167-169
: LGTM!The code changes for clearing the matured ballots are correctly implemented.
x/observer/keeper/ballot_test.go (2)
122-163
: LGTM!The
TestKeeper_DeleteBallot
function is well-structured and covers the necessary scenarios for testing the deletion of ballots. The use of sub-tests enhances readability and maintainability.
165-209
: LGTM!The
TestKeeper_DeleteBallotList
function is well-structured and covers the necessary scenarios for testing the deletion of ballot lists. The use of sub-tests enhances readability and maintainability.proto/zetachain/zetacore/observer/query.proto (1)
252-252
: Approve the non-nullable option for thevoters
field.The addition of the
(gogoproto.nullable) = false
option for thevoters
field enhances data integrity by enforcing that the list cannot contain null values. This change aligns with the goal of improving the robustness of thevoters
field.Verify the removal of the
VoterList
message.The
VoterList
message has been removed from the diff. While this change does not directly affect theQueryBallotByIdentifierResponse
message, it is important to ensure that the removal of theVoterList
message does not introduce any compatibility issues or unintended consequences in other parts of the codebase.Please confirm that the removal of the
VoterList
message is intentional and does not impact the functionality or compatibility of the system.x/observer/types/ballot_test.go (1)
482-585
: Excellent test coverage for theGenerateVoterList
method.The
TestBallot_GenerateVoterList
function is a valuable addition to the test suite. It employs a table-driven approach to test theGenerateVoterList
method of theBallot
struct, covering scenarios with success observations, failure observations, and mixed observations.The test cases are well-defined, and the expected voter lists are clearly specified. This approach enhances the readability and maintainability of the test code.
Great work on improving the test coverage!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, just couple minor comments
Small note: let's keep this one aside for now to not introduce migration scripts as we already heavy operations with gateway and Solana |
Hi @lumtis We can consider breaking the pr down, into
|
We actually want to remove all old ballot so we can easily query all active ballots |
func (k Keeper) ClearMaturedBallotsAndBallotList(ctx sdk.Context, ballots []types.Ballot, maturityBlocks int64) { | ||
for _, ballot := range ballots { | ||
k.DeleteBallot(ctx, ballot.BallotIdentifier) | ||
EmitEventBallotDeleted(ctx, ballot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we should remove it. This will just cause further state size increases right?
@@ -30,18 +42,21 @@ func (k Keeper) GetBallot(ctx sdk.Context, index string) (val types.Ballot, foun | |||
return val, true | |||
} | |||
|
|||
func (k Keeper) GetBallotList(ctx sdk.Context, height int64) (val types.BallotListForHeight, found bool) { | |||
func (k Keeper) GetBallotListForHeight(ctx sdk.Context, height int64) (val types.BallotListForHeight, found bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would just returning a types.BallotListForHeight
be enough for the caller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be fine , but this pr just renames the function ,
We can consider changing the signature in a separate PR, as the function is used in a few different places.
if !found { | ||
continue | ||
} | ||
k.DeleteBallot(ctx, ballotIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if the ballot is still pending with minority votes (e.g., 4/9)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the chances of that are quite unlikely, that is why we have maturity blocks
However, if that does happen, a new ballot would be created when the fifth vote comes in, and the four observers who already voted would need to revote. This can be triggered by adding trackers manually.
Overall though the maturity blocks should be adjusted so that it has enough time for a ballot to finalize as we do not want to keep the ballots in the system indefinitly
DeleteBallotListForHeight(ctx sdk.Context, height int64) | ||
} | ||
|
||
const MaturityBlocks = int64(100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide maturityBlocks as an arg here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maturity blocks are part of the emissions params, which makes it challenging to use the parameter.
I think using the constant here is fine for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maturity blocks are part of the emissions params
Why emission and not observer / cross-chain ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its legacy code ,and should be changed ,
But not in this PR
Description
Closes #942
The PR does the following
How Has This Been Tested?
Summary by CodeRabbit
New Features
VoterList
message type to track individual voter details.Bug Fixes
Tests
Documentation